-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(state)!: add the ability to submit blobs in parallel #4595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
state/core_access.go
Outdated
| response, err := client.SubmitPayForBlobWithAccount(ctx, account.Name(), libBlobs, opts...) | ||
| response, err := client.SubmitPayForBlob(ctx, libBlobs, opts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the bulk of the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly good w me, mostly about namings
nodebuilder/state/config.go
Outdated
| // parallel submissions. This means that blobs can be submitted by multiple different | ||
| // signers, and that blobs will not be submitted on chain in the original sending order. | ||
| // This is highly recommended for high throughput chains. | ||
| WorkerAccounts int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| WorkerAccounts int | |
| TxWorkerAccounts int |
| } | ||
|
|
||
| if cmd.Flag(txWorkerAccountsFlag).Changed { | ||
| value, err := strconv.Atoi(cmd.Flag(txWorkerAccountsFlag).Value.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can fatal out on err here
state/core_access.go
Outdated
| ca.estimatorConn = estimatorConn | ||
| } | ||
|
|
||
| if ca.workerAccounts > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe > 1 since default is 1
state/option.go
Outdated
| // WithWorkerAccounts configures the CoreAccessor to manage the provided number of | ||
| // worker accounts via the TxClient. A value of zero leaves parallel | ||
| // submission disabled. | ||
| func WithWorkerAccounts(workerAccounts int) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func WithWorkerAccounts(workerAccounts int) Option { | |
| func WithTxWorkerAccounts(workerAccounts int) Option { |
1f32639 to
12ed37e
Compare
state/core_access.go
Outdated
|
|
||
| response, err := client.SubmitPayForBlobWithAccount(ctx, account.Name(), libBlobs, opts...) | ||
| var response *user.TxResponse | ||
| if account.Name() == ca.defaultSignerAccount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is ugly, but until we have TxClient refactor, it will remain ugly.
Also, once we get Signer field in TxResponse, we should implement unit test for this. For now, manually tested.
For documentation purposes (cc @jcstein)This PR introduces parallel transaction submission lanes #5776. Parallel Transaction Submission LanesNode runners enable this feature by setting [State]
DefaultKeyName = "my_celes_key"
DefaultBackendName = "test"
EstimatorAddress = ""
EnableEstimatorTLS = false
TxWorkerAccounts = 8
Example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We should also mark this change as (config) breaking btw
|
generally correct, but I could see room for confusion
might be worth refactoring this to state the 1 thing first, as the "cannot use this new feature" thing might cause confusion. ofc, we can use this new feature and it actually provides more sequential ordering than before since we don't need to resign. The current TxClient is actually incapable of sequential ordering as is fwiw. The bigger diff here might be that the blobs come from different accounts.
imo it would clearer to state something along the lines of: Setting a value to 0 preservers the previous behavior of submitting multiple blobs from the same account. This is not recommended due to hitting many sequence mismatches and low throughput. Setting a value of 1 is breaking in that blobs are queued and submitted one by one. This avoid sequence mismatches and preserves the original order of the blobs in almost all cases. |
@evan-forbes we do not even allow a TxWorker count of 0 -- we default to 1 worker. We enforce this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve my own PR but I approve the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good. I only have a few nits
| // TxWorkerAccounts defines how many accounts the tx client manages for | ||
| // PayForBlob submissions. | ||
| // - Value of 0 submits transactions immediately (without a submission queue). | ||
| // - Value of 1 uses synchronous submission (submission queue with default | ||
| // signer as author of transactions). | ||
| // - Value of > 1 uses parallel submission (submission queue with several accounts | ||
| // submitting blobs). Parallel submission is not guaranteed to include blobs | ||
| // in the same order as they were submitted. | ||
| TxWorkerAccounts int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| // TxWorkerAccounts defines how many accounts the tx client manages for | |
| // PayForBlob submissions. | |
| // - Value of 0 submits transactions immediately (without a submission queue). | |
| // - Value of 1 uses synchronous submission (submission queue with default | |
| // signer as author of transactions). | |
| // - Value of > 1 uses parallel submission (submission queue with several accounts | |
| // submitting blobs). Parallel submission is not guaranteed to include blobs | |
| // in the same order as they were submitted. | |
| TxWorkerAccounts int | |
| // TxWorkerAccounts is used for queued submission. It defines how many accounts the | |
| // tx client uses for PayForBlob submissions. | |
| // - Value of 0 submits transactions immediately (without a submission queue). | |
| // - Value of 1 uses synchronous submission (submission queue with default | |
| // signer as author of transactions). | |
| // - Value of > 1 uses parallel submission (submission queue with several accounts | |
| // submitting blobs). Parallel submission is not guaranteed to include blobs | |
| // in the same order as they were submitted. | |
| TxWorkerAccounts int |
| "specifies how many accounts the TxClient manages for PayForBlob submission.\n"+ | ||
| "0 disables parallel submission. Values greater than 1 automatically create "+ | ||
| "\"parallel-worker-*\" accounts and grant them fees using the default signer.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: specify behaviour of when this is set to 1
| "specifies how many accounts the TxClient manages for PayForBlob submission.\n"+ | |
| "0 disables parallel submission. Values greater than 1 automatically create "+ | |
| "\"parallel-worker-*\" accounts and grant them fees using the default signer.", | |
| "is used for queued submission. It defines how many accounts the TxClient uses for PayForBlob submission.\n"+ | |
| "0 disables queued submission. 1 uses the default account. Values greater than 1 automatically create "+ | |
| "\"parallel-worker-*\" accounts and grant them fees using the default signer.", |
| ) | ||
| flags.Int( | ||
| txWorkerAccountsFlag, | ||
| 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the default? Don't we want it to be 0?
| // - Value of > 1 uses parallel submission (submission queue with several accounts | ||
| // submitting blobs). Parallel submission is not guaranteed to include blobs | ||
| // in the same order as they were submitted. | ||
| workerAccounts int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for consistency
| workerAccounts int | |
| txWorkerAccounts int |
refactor + fix(state): Rename opts + cfg param && honour submission path thru SubmitPayForBlobWithAccount if non-default account specified in TxConfig refactor(state): TxWorkerAccounts == 0 --> preserve default behaviour
01d2ba2 to
c392d26
Compare
|
Is this replaced by #4620? |
This is a draft until we get an official release from app, but it would be super useful for me to get reviews from node folks as I do not know what I'm doing in this repo 😅
Currently, node automatically submits blobs asynchronously. Meaning we can submit multiple per block, but the ordering isn't guaranteed.
This adds the ability to submit blobs in parallel and the ability to submit blobs synchronously.
Synchronous (workers == 1) has a single worker that has a queue of blobs. It submits the blob, waits for it to be confirmed, and then submits the next blob in the queue. This doesn't guarantee order, as its possible a tx fails or gets evicted and the next blob in the queue does not. However most importantly avoids sequence mismatches since we're only ever submitting a single blob from an account at once. The account that is submitting the blob here is the one passed to TxClient.
Parallel submission works in the exact same way, except there are more than one worker. In order to manage these accounts, it creates new ones and grants them a feegrant by submitting a single transaction for all accounts that don't already have this. This ofc means that different accounts will submit blobs. All accounts are in the same keyring that has the main account.